Removing events from context #596
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Conflicts: # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/Event.java
# Conflicts: # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/Operator.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java
…/java-operator-sdk into informer-creventsource
# Conflicts: # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java
# Conflicts: # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java
|
pls review this first: #581 |
# Conflicts: # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ExecutionScope.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEvent.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEvent.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEvent.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/InformerEventSource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEvent.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/TimerEventSource.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventBufferTest.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventListTest.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSourceTest.java # operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java
metacosm
reviewed
Oct 12, 2021
| } | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| private void markEvent(Event event) { |
There was a problem hiding this comment.
Shouldn't this be a method on EventMarker?
There was a problem hiding this comment.
Did not want to put it there, since Event marker should not know that there is special CustomResourceEvent . Will renamet his method so it's more descriptive.
...framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java
Outdated
Show resolved
Hide resolved
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java
Show resolved
Hide resolved
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java
Outdated
Show resolved
Hide resolved
...framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java
Outdated
Show resolved
Hide resolved
| eventMarker.markEventReceived(sampleCustomResourceID); | ||
|
|
||
| assertThat(eventMarker.getEventingState(sampleCustomResourceID)) | ||
| .isEqualTo(DELETE_AND_NON_DELETE_EVENT_PRESENT); |
There was a problem hiding this comment.
Is this situation supposed to happen? When would it?
There was a problem hiding this comment.
Good point. So if we state that delete events are retried only when if finalizer is used. Or even in more general the deleteResource method is called in case we use finalizers. This actually might not make sense. This is the case now. So we receive a delete event we don't call the deleteResource method.
Will document this on deleteResource method for now. And remove this test, also change the marker.
There was a problem hiding this comment.
Pushed changes, event marker changed quite a bit
...tor-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventMarkerTest.java
Outdated
Show resolved
Hide resolved
metacosm
reviewed
Oct 13, 2021
| @@ -6,12 +6,15 @@ | |||
| public interface ResourceController<R extends CustomResource> { | |||
|
|
|||
| /** | |||
| * Note that this method is used in combination of finalizers. If automatic finalizer handling is | |||
There was a problem hiding this comment.
Hmm, that's a significant change. So if you don't have a finalizer, you don't get a chance to clean up associated components? That seems wrong to me, the SDK should always give the users the opportunity to clean things up, regardless of finalizers.
There was a problem hiding this comment.
The thing is, that users either use finalizers or all dependent resources are cleaned up using owner reference by kubernetes. Otherwise it's not correct. Typically if the operator is down, the delete event will be missed, and cleanup would not happen anyways. So I think it is actually good to force this pattern.
| @@ -188,23 +195,29 @@ void eventProcessingFinished( | |||
| postExecutionControl); | |||
| unsetUnderExecution(executionScope.getCustomResourceID()); | |||
|
|
|||
| if (retry != null && postExecutionControl.exceptionDuringExecution()) { | |||
| // If a delete event present it was received during reconciliation. | |||
There was a problem hiding this comment.
Not sure I understand this comment… :(
There was a problem hiding this comment.
tried to rephrase it.
...framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java
Show resolved
Hide resolved
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java
Outdated
Show resolved
Hide resolved
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java
Outdated
Show resolved
Hide resolved
…tor/processing/EventMarker.java Co-authored-by: Chris Laprun <[email protected]>
…tor/processing/EventMarker.java Co-authored-by: Chris Laprun <[email protected]>
# Conflicts: # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java
metacosm
added a commit
that referenced
this pull request
Oct 28, 2021
* fix: WIP * fix: Addressing Custom Resource by Name and Namespace refactor + Informer Cache WIP * fix: Build is fixed, (test failing) * fix: Test fixes * fix: minor update * fix: EventSourceManager small fix * fix: Unit tests fixed * fix: DefaultEventHandler init from EventSourceManager * fix: custom resource selector test improvement * fix: wip test imrpovements * fix: test improvements * fix: further improvements * fix: build * feature: add mvn jar to gitignore * Exposing CustomResourceEventSource and informers * fix: cleanup * fix: remove caching optimization since it not possible anymore with informer * fix: formatting * refactor: make name/namespace final * feature: Simple label selector support * fix: formatting * fix: code inspection reports * fix: merge from v2 * fix: removed most deprecated apis * wip: started to remove events from variouse layers * fix: progress with implementation and tests * fix: Updated informer mapping to CustomResourceID * fix: imports * fix: decorational changes * fix: event marker unit test * Default Event Handler Unit tests * fix: fixes after merge * fix: changes from code review * fix: method naming * Update operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java Co-authored-by: Chris Laprun <[email protected]> * Update operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventMarker.java Co-authored-by: Chris Laprun <[email protected]> * fix: comment * fix: fixes from merge * fix: remove not used method * fix: formatting Co-authored-by: Chris Laprun <[email protected]> Co-authored-by: Chris Laprun <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.